Bug 1999734: fix(ibmcloud): Add CIS CRN to infrastructure manifest by cjschaef · Pull Request #5182 · openshift/installer

cjschaef · 2021-08-31T16:55:01Z

Bug fix for 1999734 to inject the IBM Cloud CIS Instance CRN into the infrastructure/cluster resource,
~~along with pulling in the API support for IBMCloudPlatformStatus to include the CIS CRN.~~

~~- Pull in latest IBMCloudPlatformStatus~~

Add CIS CRN to IBM Infrastructure resource

openshift-ci · 2021-08-31T16:56:09Z

@cjschaef: An error was encountered querying GitHub for users with public email (gpei@redhat.com) for bug 1999734 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message.


non-200 OK status code: 403 Forbidden body: "{\n  \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits\",\n  \"message\": \"You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\"\n}\n"

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

Details

In response to this:

Bug 1999734: fix(ibmcloud): Add CIS CRN to infrastructure manifest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rvanderp3 · 2021-08-31T17:04:15Z

/bugzilla refresh

openshift-ci · 2021-08-31T17:07:22Z

@rvanderp3: This pull request references Bugzilla bug 1999734, which is invalid:

expected the bug to target the "4.9.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cjschaef · 2021-09-09T14:18:18Z

I moved this over to 4.10, although I still cannot set Target Release

/bugzilla refresh

openshift-ci · 2021-09-09T14:18:22Z

@cjschaef: This pull request references Bugzilla bug 1999734, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

I moved this over to 4.10, although I still cannot set Target Release

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cjschaef · 2021-09-09T16:33:09Z

/bugzilla refresh

openshift-ci · 2021-09-09T16:33:13Z

@cjschaef: This pull request references Bugzilla bug 1999734, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @pamoedom

Details

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pamoedom · 2021-09-10T16:55:26Z

Hi @cjschaef, one question, this patch is designed to do an early check within the the install-config in order to verify that contains the corresponding cisInstanceCRN or just to properly abort if there is no CIS reachable within the account?
I'm asking because during my tests, when I don't include the cisInstanceCRN within the install-config, the installer proceeds anyway and gathers the CRN from the API, is this the expected behavior?
Regards.

cjschaef · 2021-09-10T17:25:30Z

@pamoedom
So, this adds the prompt (during openshift-install create manifests) to allow one to be selected (allowing multiple to be available within the designated account), which is then automatically injected in the necessary manifests (infrastructure primarily).

As far as performing installations using a predefined install-config, without the CIS CRN, that part was likely overlooked. As the CRN needs to be injected into the infrastructure/cluster resource in order to result in a successful deployment as it is required via the ingress-operator.

I will see if validation can make sure that value is in the installconfig, since it is a hard requirement for ingress-operator.

cjschaef · 2021-09-10T17:25:38Z

/hold

cjschaef · 2021-09-10T18:43:46Z

@pamoedom
Sorry, I think I misunderstood your statement above. I did some testing to create manifests using an install-config (versus the standard prompts) to see what you mean about the CIS CRN being collected.

As I stated, the CIS CRN is a requirement for ingress-operator, so without it, the IPI installation would eventually fail.
However, as long as a BaseDomain is provided, the installer should be able to look up the CRN

installer/pkg/asset/installconfig/ibmcloud/metadata.go

Lines 51 to 75 in 422e171

    
           func (m *Metadata) CISInstanceCRN(ctx context.Context) (string, error) { 
        
           	m.mutex.Lock() 
        
           	defer m.mutex.Unlock() 
        
           	if m.cisInstanceCRN == "" { 
        
           		client, err := m.Client() 
        
           		if err != nil { 
        
           			return "", err 
        
           		} 
        
           		zones, err := client.GetDNSZones(ctx) 
        
           		if err != nil { 
        
           			return "", err 
        
           		} 
        
           		for _, z := range zones { 
        
           			if z.Name == m.BaseDomain { 
        
           				m.SetCISInstanceCRN(z.CISInstanceCRN) 
        
           				return m.cisInstanceCRN, nil 
        
           			} 
        
           		} 
        
           		return "", fmt.Errorf("cisInstanceCRN unknown due to DNS zone %q not found", m.BaseDomain) 
        
           	} 
        
           	return m.cisInstanceCRN, nil 
        
           }

So, I think the CRN value should be available, based off the selected (prompted) or pre-configured BaseDomain, which would allow it to be injected into the infrastructure/cluster resource, for ingress-operator. Which is what this PR is specifically attempting to fix. If the CRN cannot be found for some reason, that would be an issue and should result in an error.

cjschaef · 2021-09-10T18:46:46Z

Per testing with/without install-config, the CIS CRN is successfully looked up, provided a valid baseDomain is selected/exists (errors are returned otherwise).
That CRN is properly injected into the infrastructure/cluster resource as required.

/hold cancel

pamoedom · 2021-09-13T11:50:28Z

Thanks @cjschaef, more clear now, my confusion was the use of cisInstanceCRN within the install-config.yaml but I can see now that is not really needed, the CIS lookup will only depend on the baseDomain.
Let me do some tests from my side to corroborate the behaviour in all scenarios, I'll keep you posted.

pamoedom · 2021-09-13T13:43:12Z

Here you have my test results:

[Without baseDomain but using cisInstanceCRN]

$ ./openshift-install-local create manifests --dir bz1999734_nodomain_cis/ --log-level debug
DEBUG OpenShift Installer unreleased-master-4958-g3ecc5db80828cb596b63f02582533fc8c5ec9c10 
DEBUG Built from commit 3ecc5db80828cb596b63f02582533fc8c5ec9c10 
DEBUG Fetching Master Machines...                  
DEBUG Loading Master Machines...                   
DEBUG   Loading Cluster ID...                      
DEBUG     Loading Install Config...                
DEBUG       Loading SSH Key...                     
DEBUG       Loading Base Domain...                 
DEBUG         Loading Platform...                  
DEBUG       Loading Cluster Name...                
DEBUG         Loading Base Domain...               
DEBUG         Loading Platform...                  
DEBUG       Loading Networking...                  
DEBUG         Loading Platform...                  
DEBUG       Loading Pull Secret...                 
DEBUG       Loading Platform...                    
FATAL failed to fetch Master Machines: failed to load asset "Install Config": invalid "install-config.yaml" file: baseDomain: Invalid value: "": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

[With correct baseDomain but using a wrong cisInstanceCRN]

$ ./openshift-install-local create manifests --dir bz1999734_domain_badcis/ --log-level debug
DEBUG OpenShift Installer unreleased-master-4958-g3ecc5db80828cb596b63f02582533fc8c5ec9c10 
DEBUG Built from commit 3ecc5db80828cb596b63f02582533fc8c5ec9c10 
DEBUG Fetching Master Machines...                  
DEBUG Loading Master Machines...                   
DEBUG   Loading Cluster ID...                      
DEBUG     Loading Install Config...                
DEBUG       Loading SSH Key...                     
DEBUG       Loading Base Domain...                 
DEBUG         Loading Platform...                  
DEBUG       Loading Cluster Name...                
DEBUG         Loading Base Domain...               
DEBUG         Loading Platform...                  
DEBUG       Loading Networking...                  
DEBUG         Loading Platform...                  
DEBUG       Loading Pull Secret...                 
DEBUG       Loading Platform...                    
[...]   
DEBUG Generating Openshift Manifests...            
INFO Manifests created in: bz1999734_domain_badcis/manifests and bz1999734_domain_badcis/openshift 

$ grep -nr "cis" bz1999734_domain_badcis/*
bz1999734_domain_badcis/manifests/cluster-infrastructure-02-config.yml:22:      cisInstanceCRN: 'crn:v1:bluemix:public:internet-svcs:global:xxx::'

NOTE: the manifest is created with the correct cisInstanceCRN from the account.

[With wrong baseDomain but correct cisInstanceCRN]

$ ./openshift-install-local create manifests --dir bz1999734_baddomain_cis/ --log-level debug
DEBUG OpenShift Installer unreleased-master-4958-g3ecc5db80828cb596b63f02582533fc8c5ec9c10 
DEBUG Built from commit 3ecc5db80828cb596b63f02582533fc8c5ec9c10 
DEBUG Fetching Master Machines...                  
DEBUG Loading Master Machines...                   
DEBUG   Loading Cluster ID...                      
DEBUG     Loading Install Config...                
DEBUG       Loading SSH Key...                     
DEBUG       Loading Base Domain...                 
DEBUG         Loading Platform...                  
DEBUG       Loading Cluster Name...                
DEBUG         Loading Base Domain...               
DEBUG         Loading Platform...                  
DEBUG       Loading Networking...                  
DEBUG         Loading Platform...
[...]
DEBUG   Fetching DNS Config...                     
DEBUG     Fetching Install Config...               
DEBUG     Reusing previously-fetched Install Config 
DEBUG     Fetching Cluster ID...                   
DEBUG     Reusing previously-fetched Cluster ID    
DEBUG     Fetching Platform Credentials Check...   
DEBUG     Reusing previously-fetched Platform Credentials Check 
DEBUG   Generating DNS Config...                   
FATAL failed to fetch Common Manifests: failed to fetch dependency of "Common Manifests": failed to generate asset "DNS Config": failed ot get DNS zone ID: DNS zone "imbcloud.qe.devcluster.openshift.com" not found

Summary: no matter what you try to hardcode using cisInstanceCRN within the install-config.yaml, that variable will depend from the proper lookup of the baseDomain within the account, which means the error message provided by this PR will not be easy to trigger unless some unexpected error occurs between the installer and the ibmcloud API.

@cjschaef, do my conclusions makes sense? any other scenario we can test to force that error to appear? AFAIK, no more that one CIS can be available per account and no external domain can be active without the proper CIS instance, right?

cjschaef · 2021-09-13T14:50:18Z

@pamoedom That all makes sense to me. At the moment, I would think the only way to get to that error is if the CIS instance and/or domain were removed at exactly the right time. I do believe you can have multiple Cloud Internet Services for an IBM Cloud account, but you are correct that a domain cannot be active without the backing CIS.

However, the main target of this PR was to add this line, injecting the CIS CRN into the infrastructure/cluster resource.
https://github.com/openshift/installer/pull/5182/files#diff-306ba1253c4b1cd58fb44dbca55765f1aa8a2bea356a3a095070c327a9a2880aR182

So, while retrieving that value, I assumed it would be best to handle the error from the function used, supplying this error message in such a case.
https://github.com/openshift/installer/pull/5182/files#diff-306ba1253c4b1cd58fb44dbca55765f1aa8a2bea356a3a095070c327a9a2880aR175

But if there is a better function or way to get the CIS CRN, I'd be open to using that so as not to have to check the potential error returned by the function I propose to use here.

pamoedom · 2021-09-13T16:10:19Z

The part of injecting the cisInstanceCRN within the manifests is working as expected:

$ cat bz1999734_domain_badcis/manifests/cluster-infrastructure-02-config.yml 
apiVersion: config.openshift.io/v1
kind: Infrastructure
metadata:
  creationTimestamp: null
  name: cluster
spec:
  cloudConfig:
    key: config
    name: cloud-provider-config
  platformSpec:
    type: IBMCloud
status:
  apiServerInternalURI: https://api-int.pamoedo-test.ibmcloud.qe.devcluster.openshift.com:6443
  apiServerURL: https://api.pamoedo-test.ibmcloud.qe.devcluster.openshift.com:6443
  controlPlaneTopology: HighlyAvailable
  etcdDiscoveryDomain: ""
  infrastructureName: pamoedo-test-fjmgd
  infrastructureTopology: HighlyAvailable
  platform: IBMCloud
  platformStatus:
    ibmcloud:
      cisInstanceCRN: 'crn:v1:bluemix:public:internet-svcs:global:<obfuscated>::'
      location: eu-de
      resourceGroupName: pamoedom-rg
    type: IBMCloud

Regarding alternatives to get the CIS I'm afraid I cannot help much with that, but IMHO, having an extra error check in place for unexpected scenarios is always a good idea, looks good to me.

pamoedom · 2021-09-13T16:11:02Z

/lgtm
/label qe-approved
/bugzilla cc-qa

openshift-ci · 2021-09-13T16:11:06Z

@pamoedom: This pull request references Bugzilla bug 1999734, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @pamoedom

Details

In response to this:

/lgtm
/label qe-approved
/bugzilla cc-qa

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cjschaef · 2021-10-01T14:50:19Z

Dropped the API update, as it has been updated to a more recent version already, which should have the support needed for the changes in this PR.

openshift-ci · 2021-10-01T17:01:44Z

@cjschaef: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-openstack-provider-network	3ecc5db80828cb596b63f02582533fc8c5ec9c10	link	`/test e2e-openstack-provider-network`
ci/prow/e2e-aws-workers-rhel7	`6b461c7`	link	false	`/test e2e-aws-workers-rhel7`
ci/prow/e2e-aws-workers-rhel8	`6b461c7`	link	false	`/test e2e-aws-workers-rhel8`
ci/prow/e2e-libvirt	`6b461c7`	link	false	`/test e2e-libvirt`
ci/prow/e2e-crc	`6b461c7`	link	false	`/test e2e-crc`
ci/prow/e2e-metal-single-node-live-iso	`6b461c7`	link	false	`/test e2e-metal-single-node-live-iso`
ci/prow/e2e-aws-single-node	`6b461c7`	link	false	`/test e2e-aws-single-node`
ci/prow/e2e-metal-ipi-ovn-ipv6	`6b461c7`	link	false	`/test e2e-metal-ipi-ovn-ipv6`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

cjschaef · 2021-10-04T14:09:30Z

/retest-required

jstuever · 2021-10-05T19:13:49Z

/cc

pamoedom

/lgtm

jstuever · 2021-10-07T16:05:03Z

/approve

openshift-ci · 2021-10-07T16:05:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jstuever

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [jstuever]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2021-10-07T16:18:43Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-07T16:57:41Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-07T18:41:43Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

staebler · 2021-10-07T19:26:39Z

/hold

Some of the descriptions in the comments concern me. Give me a chance to look this over.

staebler · 2021-10-07T19:45:50Z

The following part of #5182 (comment) is what concerned me. I was concerned that we were adding behavior that would override the CRN explicitly specified by the user in the install-config.yaml. However, I do not see any field in the install config where the user can specify the CRN, so perhaps the comment is out-dated.

/hold cancel

Summary: no matter what you try to hardcode using cisInstanceCRN within the install-config.yaml, that variable will depend from the proper lookup of the baseDomain within the account, which means the error message provided by this PR will not be easy to trigger unless some unexpected error occurs between the installer and the ibmcloud API.

openshift-bot · 2021-10-07T19:46:47Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-07T19:59:43Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-10-07T20:02:53Z

@cjschaef: All pull requests linked via external trackers have merged:

openshift/installer#5182

Bugzilla bug 1999734 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1999734: fix(ibmcloud): Add CIS CRN to infrastructure manifest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Add the CIS CRN to infrastructure manifest for Power VS provider. This is required for the cluster-ingress-operator. This PR does what openshift#5182 does for IBM Cloud, but for Power VS.

openshift-ci Bot requested review from e-tienne and patrickdillon August 31, 2021 17:01

openshift-ci Bot added the bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Aug 31, 2021

openshift-ci Bot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Aug 31, 2021

openshift-ci Bot removed the bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Sep 9, 2021

openshift-ci Bot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Sep 9, 2021

openshift-ci Bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 9, 2021

openshift-ci Bot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 9, 2021

openshift-ci Bot requested a review from pamoedom September 9, 2021 16:33

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2021

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2021

openshift-ci Bot added the qe-approved Signifies that QE has signed off on this PR label Sep 13, 2021

openshift-ci Bot requested a review from pamoedom October 1, 2021 14:49

openshift-ci Bot requested a review from jstuever October 5, 2021 19:13

mjturek mentioned this pull request Oct 6, 2021

Power VS: Add CIS CRN to Infrastructure manifest #5269

Closed

pamoedom reviewed Oct 7, 2021

View reviewed changes

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Oct 7, 2021

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 7, 2021

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2021

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2021

openshift-merge-robot merged commit 5a50e36 into openshift:master Oct 7, 2021

cjschaef deleted the bz_1999734 branch October 7, 2021 20:04

mjturek mentioned this pull request Nov 3, 2021

Add CIS CRN to Infrastructure manifest openshift-powervs/installer#67

Merged

Conversation

cjschaef commented Aug 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Aug 31, 2021

Uh oh!

rvanderp3 commented Aug 31, 2021

Uh oh!

openshift-ci Bot commented Aug 31, 2021

Uh oh!

cjschaef commented Sep 9, 2021

Uh oh!

openshift-ci Bot commented Sep 9, 2021

Uh oh!

cjschaef commented Sep 9, 2021

Uh oh!

openshift-ci Bot commented Sep 9, 2021

Uh oh!

pamoedom commented Sep 10, 2021

Uh oh!

cjschaef commented Sep 10, 2021

Uh oh!

cjschaef commented Sep 10, 2021

Uh oh!

cjschaef commented Sep 10, 2021

Uh oh!

cjschaef commented Sep 10, 2021

Uh oh!

pamoedom commented Sep 13, 2021

Uh oh!

pamoedom commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjschaef commented Sep 13, 2021

Uh oh!

pamoedom commented Sep 13, 2021

Uh oh!

pamoedom commented Sep 13, 2021

Uh oh!

openshift-ci Bot commented Sep 13, 2021

Uh oh!

cjschaef commented Oct 1, 2021

Uh oh!

openshift-ci Bot commented Oct 1, 2021

Uh oh!

cjschaef commented Oct 4, 2021

Uh oh!

jstuever commented Oct 5, 2021

Uh oh!

pamoedom left a comment

Choose a reason for hiding this comment

Uh oh!

jstuever commented Oct 7, 2021

Uh oh!

openshift-ci Bot commented Oct 7, 2021

Uh oh!

openshift-bot commented Oct 7, 2021

Uh oh!

openshift-bot commented Oct 7, 2021

Uh oh!

openshift-bot commented Oct 7, 2021

Uh oh!

staebler commented Oct 7, 2021

Uh oh!

staebler commented Oct 7, 2021

Uh oh!

openshift-bot commented Oct 7, 2021

Uh oh!

openshift-bot commented Oct 7, 2021

Uh oh!

openshift-ci Bot commented Oct 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

cjschaef commented Aug 31, 2021 •

edited

Loading

pamoedom commented Sep 13, 2021 •

edited

Loading